Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

doc: document readline keybindings #20825

Closed

Conversation

shobhitchittora
Copy link
Contributor

Closes: #20814

  • make -j4 test (UNIX), or vcbuild test (Windows) passes
  • tests and/or benchmarks are included
  • documentation is changed or added
  • commit message follows commit guidelines

Affected subsystem(s)

doc

@nodejs-github-bot nodejs-github-bot added the doc Issues and PRs related to the documentations. label May 18, 2018
doc/api/tty.md Outdated
@@ -158,7 +158,109 @@ The `tty.isatty()` method returns `true` if the given `fd` is associated with
a TTY and `false` if it is not, including whenever `fd` is not a non-negative
integer.

## TTY keybindings
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Can we document these in the readline doc? While a TTY is required for most of these, they shouldonly affect readline behaviour

Copy link
Member

@BridgeAR BridgeAR May 19, 2018

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I guess it would be fine to add a reference to those in the repl and in here.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@addaleax @BridgeAR just to be clear, I'll add the table of keybindings in the readline doc and ref the table in REPL and TTY doc.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@shobhitchittora Yes, that’s the idea (although I don’t really understand why this would be added to the TTY doc)

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@addaleax I saw this code in readline -

Interface.prototype.write = function(d, key) {
  if (this.paused) this.resume();
  this.terminal ? this._ttyWrite(d, key) : this._normalWrite(d);
};

and thought that TTY case need a doc. But yeah we can have the table in readline doc and ref it in REPL and / or TTY.

Need you opinion on this.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yeah, exactly – it’s code in readline. :) We should point out that these codes apply only for TTYs, but it’s not something that’s interesting for code that uses TTYs in general.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@addaleax you are right, the TTY docs are actually the wrong place for this. The repl should have a reference however.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Done.

@vsemozhetbyt vsemozhetbyt added the readline Issues and PRs related to the built-in readline module. label May 19, 2018
@richardlau
Copy link
Member

Nit: handled is misspelled in the commit message. Alternatively maybe use the shorter doc: document readline keybindings.

@shobhitchittora shobhitchittora changed the title doc: adds handeled tty write keybindings doc: document readline keybindings May 20, 2018
@@ -287,7 +287,8 @@ added: v0.1.98

The `rl.write()` method will write either `data` or a key sequence identified
by `key` to the `output`. The `key` argument is supported only if `output` is
a [TTY][] text terminal.
a [TTY][] text terminal. See [`TTY keybindings`][] for a list of covered key
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Nit: remove "covered"?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Done.

1. Adds ref in repl doc
2. Removes ref from tty doc

Closes: nodejs#20814
@shobhitchittora
Copy link
Contributor Author

@addaleax @Trott is TTY keybindings fine or the bindings should be named Special keys?

doc/api/repl.md Outdated
@@ -634,3 +636,4 @@ For an example of running a REPL instance over [curl(1)][], see:
[`util.inspect()`]: util.html#util_util_inspect_object_options
[curl(1)]: https://curl.haxx.se/docs/manpage.html
[stream]: stream.html
[`TTY keybindings`]: readline.html#readline_tty_keybindings
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It seems we do not need backticks in this case (this is not a code fragment). So this should be [TTY keybindings] and go before the [curl(1)]: (as per ASCII sorting we use in reference lists).

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

done.

doc/api/tty.md Outdated
@@ -161,4 +161,4 @@ integer.
[`net.Socket`]: net.html#net_class_net_socket
[`process.stdin`]: process.html#process_process_stdin
[`process.stdout`]: process.html#process_process_stdout
[`process.stderr`]: process.html#process_process_stderr
[`process.stderr`]: process.html#process_process_stderr
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Missing line break at the end of the file?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

done

doc/api/repl.md Outdated
@@ -67,6 +67,8 @@ The following key combinations in the REPL have these special effects:
variables. When pressed while entering other input, displays relevant
autocompletion options.

For a full list of special keys, refer [`TTY keybindings`][].
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

refer -> refer to?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

done

@@ -535,3 +637,4 @@ rl.on('line', (line) => {
[TTY]: tty.html
[Writable]: stream.html#stream_writable_streams
[reading files]: #readline_example_read_file_stream_line_by_line
[`TTY keybindings`]: #readline_tty_keybindings
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It seems we do not need backticks in this case (this is not a code fragment). So this should be [TTY keybindings] and go after the [TTY]: (as per ASCII sorting we use in reference lists).

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

done

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We need the same fix in the text)

</tr>
<tr>
<td><code>ctrl+c</code></td>
<td>emits SIGINT</td>
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

emits -> emit for consistency with other cells?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

done

</tr>
<tr>
<td><code>ctrl+a</code></td>
<td>goto start of line</td>
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

goto -> go to?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

done

</tr>
<tr>
<td><code>ctrl+e</code></td>
<td>goto to end of line</td>
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

goto to -> go to?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

done

</tr>
<tr>
<td><code>ctrl+p</code></td>
<td>prev history item </td>
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

prev -> previous for more formal style?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

done.

@vsemozhetbyt
Copy link
Contributor

Should we note that some bindings are OS-dependent? For example, these seem to not work for me on Windows as documented (tested in REPL): ctrl+shift+backspace, ctrl+u, ctrl+backspace, meta+delete (i.e. win+delete).

1. typo fix
2. adding new lines

Closes: nodejs#20814
@shobhitchittora
Copy link
Contributor Author

@vsemozhetbyt I think it'd be great if we can do that. Unfortunately I don't have a windows system and cannot verify bindings for the same. All help is appreciated. 😇

doc/api/repl.md Outdated
[curl(1)]: https://curl.haxx.se/docs/manpage.html
[stream]: stream.html

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This empty line seems unneeded)

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

done.

@vsemozhetbyt
Copy link
Contributor

cc @nodejs/platform-windows: can we have some more verification of keybindings and suggestions how to better document possible differences (if we should)?

@bzoz
Copy link
Contributor

bzoz commented May 24, 2018

Meta is the Alt key (maybe we should add that to the docs?), bindings with those work.

The ctrl+shift+... don't work on Windows and also do not work on WSL.

@vsemozhetbyt
Copy link
Contributor

@bzoz Sorry, was confused by the MDN page.

But alt+delete still does not work for me (alt+d works).

Copy link
Member

@BridgeAR BridgeAR left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM with the whitespace removed. My two comments would also be nice but I think that could also be done later on (even though I would prefer to have it that way right away).

</tr>
<tr>
<td><code>ctrl+shift+backspace</code></td>
<td>delete line left</td>
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Not a must but I personally would prefer the descriptions all to start with a upper case character.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

done.

<th>Description</th>
</tr>
<tr>
<td><code>ctrl+shift+backspace</code></td>
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggestion: add whitespace between the individual commands and maybe also only highlight the keys, not the plus. As in: <code>ctrl</code> + <code>shift</code> + <code>backspace</code>

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Any reason these are wrapped in <code> and not <kbd>?

@bzoz
Copy link
Contributor

bzoz commented May 24, 2018

Hm.. Alt+Del does not work on Windows and WSL, but works on Linux. Libuv reports it as ^^[3~ on Windows.

Ctrl + Shift + Backspace does not work neither on Windows nor on Linux. On Windows libuv does not report any sequence at all.

Ctrl + Shift + Del works on both Windows and Linux.

</tr>
<tr>
<td><code>ctrl</code> + <code>z</code></td>
<td>(need clarification)</td>
Copy link
Contributor Author

@shobhitchittora shobhitchittora May 24, 2018

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hey team.
Need clarification on this keybinding. I cannot think of a description for this. Can someone help with this?

cc @BridgeAR @vsemozhetbyt

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I do not know, sorry(

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@BridgeAR I found this for ctrl + z -
https://nodejs.org/api/readline.html#readline_event_sigtstp

If it's fine we can either ref to this or take the first paragraph and add here.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Please document it as e.g.: Moves running process into background. Type 'fg' and press 'enter' to return..

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

done.

@vsemozhetbyt
Copy link
Contributor

@vsemozhetbyt
Copy link
Contributor

We have a linter issue:

06:00:06 Running Markdown linter on docs...
06:00:51 doc/api/readline.md
06:00:51   596:113  warning  Line must be at most 80 characters  maximum-line-length  remark-lint
06:00:51   599:108  warning  Line must be at most 80 characters  maximum-line-length  remark-lint
06:00:51   623:105  warning  Line must be at most 80 characters  maximum-line-length  remark-lint

<td>Moves running process into background. Type <code>fg</code> and press <code>enter</code> to return.</td>
</tr>
<tr>
<td><code>ctrl</code> + <code>w</code> &nbsp; or &nbsp; <code>ctrl</code> + <code>backspace</code></td>
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Are these &nbsp; intended here and below?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yeah! added them for more spacing, but removing for now.

@vsemozhetbyt
Copy link
Contributor

@vsemozhetbyt
Copy link
Contributor

Should we land as is or need we elaborate some note about OS-dependent differences?

@bzoz
Copy link
Contributor

bzoz commented May 27, 2018

I think we should document differences. And double check, since it looks like Ctrl + Shift + Backspace does not work on any platform.

@vsemozhetbyt
Copy link
Contributor

Let's do not stall this PR.

@shobhitchittora
Copy link
Contributor Author

@vsemozhetbyt please do let know if anything update / commit is required from my side.

@vsemozhetbyt
Copy link
Contributor

@nodejs/platform-macos Are there any additions from your team?

@vsemozhetbyt
Copy link
Contributor

ping @nodejs/documentation

@shobhitchittora
Copy link
Contributor Author

@vsemozhetbyt any updates on this?

@vsemozhetbyt
Copy link
Contributor

Sorry, it seems I've exhausted all the ways I could help(

@jasnell jasnell added the stalled Issues and PRs that are stalled. label Sep 10, 2018
</tr>
<tr>
<td><code>ctrl</code> + <code>left</code></td>
<td>Word left </td>
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

On macOS, it's command+option+left. ctrl+left does not work.

</tr>
<tr>
<td><code>ctrl</code> + <code>right</code></td>
<td>Word right</td>
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

On macOS, it's command+option+right. ctrl+right does not work.

</tr>
<tr>
<td><code>meta</code> + <code>backspace</code></td>
<td>Delete word left </td>
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Does not work on macOS.

@Trott
Copy link
Member

Trott commented Nov 20, 2018

@shobhitchittora @vsemozhetbyt I've tested on my macOS laptop and have put the result in above as best I can. ("Meta" on macOS is "hold down the ESC key" and "backspace" is "fn + delete", in case that's relevant.)

Can that information be incorporated and this unstalled? Does anything else need to happen to unstall this?

@Trott Trott removed the stalled Issues and PRs that are stalled. label Nov 20, 2018
@Trott
Copy link
Member

Trott commented Nov 20, 2018

Optimistically removing the stalled label...

@Trott
Copy link
Member

Trott commented Nov 30, 2018

@shobhitchittora Are you planning to finish this one? Or would it make sense to put a help wanted label on it or something like that?

@Trott Trott added the help wanted Issues that need assistance from volunteers or PRs that need help to proceed. label Dec 4, 2018
@fhinkel
Copy link
Member

fhinkel commented Oct 26, 2019

Closing this due to inactivity. Please reopen if needed.

@fhinkel fhinkel closed this Oct 26, 2019
@BridgeAR BridgeAR mentioned this pull request Dec 21, 2019
4 tasks
HarshithaKP added a commit to HarshithaKP/node that referenced this pull request Jan 8, 2020
This is a rework of closed pr nodejs#20825
fixes: nodejs#20814
@HarshithaKP HarshithaKP mentioned this pull request Jan 8, 2020
4 tasks
BridgeAR pushed a commit that referenced this pull request Jan 13, 2020
This documents all readline key bindings. It is a rework of
#20825

PR-URL: #31256
Fixes: #20814
Reviewed-By: James M Snell <[email protected]>
Reviewed-By: Ruben Bridgewater <[email protected]>
MylesBorins pushed a commit that referenced this pull request Jan 16, 2020
This documents all readline key bindings. It is a rework of
#20825

PR-URL: #31256
Fixes: #20814
Reviewed-By: James M Snell <[email protected]>
Reviewed-By: Ruben Bridgewater <[email protected]>
codebytere pushed a commit that referenced this pull request Mar 14, 2020
This documents all readline key bindings. It is a rework of
#20825

PR-URL: #31256
Fixes: #20814
Reviewed-By: James M Snell <[email protected]>
Reviewed-By: Ruben Bridgewater <[email protected]>
targos pushed a commit to targos/node that referenced this pull request Apr 25, 2020
This documents all readline key bindings. It is a rework of
nodejs#20825

PR-URL: nodejs#31256
Fixes: nodejs#20814
Reviewed-By: James M Snell <[email protected]>
Reviewed-By: Ruben Bridgewater <[email protected]>
targos pushed a commit that referenced this pull request Apr 28, 2020
This documents all readline key bindings. It is a rework of
#20825

PR-URL: #31256
Fixes: #20814
Reviewed-By: James M Snell <[email protected]>
Reviewed-By: Ruben Bridgewater <[email protected]>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
doc Issues and PRs related to the documentations. help wanted Issues that need assistance from volunteers or PRs that need help to proceed. readline Issues and PRs related to the built-in readline module.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

doc: add doc for tty writes ( readline.js / REPL )
10 participants